Repository navigation
Avoid whole-file copies for positive-length File reads - #33
Conversation
284km
left a comment
284km
left a comment
There was a problem hiding this comment.
Reviewed 5334d5a against 23ce853. No actionable findings in this change.
The regular-file fast path preserves the existing path-based model and advances the logical position by the bytes actually returned. I checked its integration with pushback, pending-write flushing, EOF, output buffers, encoding handling, and the newline/device fallbacks.
Validation:
- Built the head at O2 with Mere
991f299band the pinned mgz revision; compared with the existing O2 base build using the same compiler and dependency revision. - The new regression matched Ruby 4.0.9 on base and head with hash indexing enabled/disabled, both normally and with
MERE_RUBY_GC_STRESS=100(eight runs). - Additional probes matched Ruby in the same eight combinations: reads around 64 KiB and EOF, output-buffer identity/encoding, symlink paths, pushback spanning a refill, truncation/regrowth between reads, BOM handling, and UTF-16 byte reads.
- Ten existing corpus programs covering IO, stream checks, flushing, and encodings matched Ruby on the head with indexing enabled/disabled (20 runs).
- One comparison using the existing 1 MiB
file_read_checks.rbbenchmark produced identical stdout on Ruby, base, and head. The foreach portion measured 16.68 s before and 15.55 s after. Other validation ran concurrently, so this single run is indicative only. - CI for this exact head reports 509/509 corpus programs matching Ruby and a successful Linux build. I did not repeat the full corpus or run Linux locally; broader CI shards remain pending.
git diff --checkpassed.
5334d5a to
a452936
Compare
284km
left a comment
284km
left a comment
There was a problem hiding this comment.
Re-reviewed rebased head a452936 against 2004502. No actionable findings.
The positioned-read runtime and regression have the same stable patch ID as the previously reviewed 5334d5a. I also checked their integration with the buffered-write change from #32 and the parser changes now present in the base.
Validation:
- Fresh O2 build with the pinned Mere
991f299bcompiler and mgz revision. - Eight focused corpus programs matched Ruby 4.0.9 with indexing ON/OFF, covering positioned reads, buffered writes, IO encodings, and the newly included parser regressions (16 runs).
- The positioned-read regression also matched with
MERE_RUBY_GC_STRESS=100in both index modes. - CI for this exact head reports 513/513 corpus programs matching Ruby and a successful Linux build. Broader jobs remain pending.
git diff --checkpassed.
The original review remains applicable to this rebase; the full corpus and performance measurements were not repeated locally.
Positive-length reads of ordinary files currently load the whole file before slicing the requested range. Read from the logical position in bounded 64 KiB chunks instead, so repeated small reads do not repeatedly copy unrelated file contents. Preserve the existing path-based model, flushing, pushback, EOF, and universal-newline fallback.
Validation:
Rebased onto main
2004502after #32 merged, preserving both benchmark descriptions. The runtime and regression patches are unchanged from reviewed5334d5a(identical stable patch IDs). Eight focused corpus programs, covering positioned reads, buffered writes, IO encodings, and the newly merged parser changes, match Ruby on the rebased head with indexing ON/OFF. The rebased head is Ready for review; its CI is running again.